fix: enforce one-issue-per-PR across miners in mirror issue discovery - #822
Conversation
844a15c to
e460e02
Compare
e460e02 to
46c20e5
Compare
anderdc
left a comment
anderdc
left a comment
There was a problem hiding this comment.
Functionally correct, but more re-architecting than the bug needs. The minimum fix is: hoist the dedup decision to round scope. Everything else (the NamedTuple, the phase-1 partition) is incidental.
Concrete scope-down ask:
-
Drop
_MinerBatch. Phase 1 just collectspending: List[Tuple[MinerEvaluation, List[MirrorIssue], int]](eval, filtered, open_issue_count) — same as today, minus the inline classify loop. -
_build_canonical_pr_owners(pending)classifies inline:for evaluation, filtered, _ in pending: for issue in filtered: if _classify_issue(issue) != 'solved': continue sp = issue.solving_pr if issue.author_github_id == sp.author_github_id: continue ...Yes, `_classify_issue` runs twice per issue (here + in the scorer). It's attribute checks — microseconds at miner scale. Not worth a NamedTuple.
-
`_score_miner_mirror_issues` keeps its current signature and inline classification path. The only change is replacing `pr_scored_keys: Set[Tuple[str, int]] = set()` and the `pr_key in pr_scored_keys` check with the marker comparison against `canonical_pr_owners`.
-
Pull `_FAR_FUTURE = datetime.max.replace(tzinfo=timezone.utc)` to module scope so the scorer and canonical builder share it instead of redefining.
Net diff should be ~50 lines instead of ~140, and the scorer barely changes — which is what you want for a scoring-economy fix where the diff itself is the audit surface.
Tests stay as-is; they target `_build_canonical_pr_owners` and end-to-end behavior, both unchanged by the refactor.
PR #796 inlined the one-issue-per-PR set inside _score_miner_mirror_issues, making the rule per-miner rather than per-round. A solving PR closing issues authored by multiple miners awarded discovery score to each miner — silently inflating issue-discovery emissions on the common 'closes #N closes #M' GitHub pattern and enabling cheap two-account collusion. Restore legacy round-global semantics by deferring scoring until every miner's batch is fetched, then resolving canonical (repo, pr_number) ownership across miners via _build_canonical_pr_owners. The earliest-created qualifying issue across all miners owns the slot; siblings on the same PR (including a single miner's later issues) count for credibility only. Same-account issues never claim the slot, mirroring legacy ordering.
5c45d0c to
6e2e83e
Compare
|
@anderdc please review the update |
2 participants
Summary
PR #796 inlined the one-issue-per-PR dedup set inside
_score_miner_mirror_issues(called once per miner), shrinking the rule's lifetime from "round" to "single miner." A solving PR closing issues authored by multiple miners then paid each one — silently inflating discovery emissions oncloses #N closes #MPRs and enabling two-account collusion.This PR restores legacy round-global semantics by:
run_mirror_issue_discoverycollects every miner's(evaluation, filtered_issues, open_issue_count)before scoring, so the one-issue-per-PR decision sees all miners at once._build_canonical_pr_ownershelper produces(repo, pr_number) → (created_at, issue_number, uid)for the earliest-created qualifying issue across all miners._score_miner_mirror_issues, the per-minerpr_scored_keysset and itspr_key in pr_scored_keys/pr_scored_keys.add(pr_key)pair are replaced with an exact marker-tuple comparison against the canonical map.Same-account issues (discoverer == solver) never claim the slot, mirroring the legacy
pr_scored.addordering._FAR_FUTUREis hoisted to module scope so the canonical builder and scorer share it. The scorer's signature gains onlycanonical_pr_owners; its classification path and counters are unchanged. Public API, log shapes, and cache stats are unchanged — only which miner pockets the score on a shared PR changes.Related Issues
Closes #821
Type of Change
Testing
Checklist